Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix dots pattern on mobile log in screen & thank you screen #240

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Fix dots pattern on mobile log in screen & thank you screen #240

merged 4 commits into from
Oct 18, 2023

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Oct 11, 2023 •
edited
Loading

Fixes #226

This PR first removes the style from the old design. Then it also ensures the distance between the dot pattern and the text is between 30-50 (submit page) or 30 & 70 40-60 (Login & Thank you screen).

Additionally, because the Cover Block has an automatically changing margin-top, it makes the CSS control of the distance more complicated on different screens (On smaller screens, the margin-top of the cover disappears, causing the text and dots pattern to overlap.). Plus, I don't see the necessity of the Cover Block here as we only have text at the moment. It might also be a part of the old design. So it has been removed.

Figma

Screenshots

Submit Page (margin bottom: 30px-50px) Login Page (30px & 70px 40px-60px) Thank You Page (30px & 70px 40px-60px)
Bigger Screen Screen Shot 2023-10-12 at 1 53 37 AM Screen Shot 2023-10-12 at 1 52 57 AM Screen Shot 2023-10-12 at 1 53 52 AM
Mobile Screen Screen Shot 2023-10-12 at 1 54 28 AM Screen Shot 2023-10-12 at 1 54 50 AM Screen Shot 2023-10-12 at 1 54 12 AM

Note

Currently, the margin between the dot pattern and text on the Login Page and Thank You Page is:

30px in the mobile view and 70px in non-mobile views (> 599px).

This is according to the Figma design. However, if we want a progressive adjustment like the Submit Page (using clamp to vary the margin between 30-50px), the Login Page and Thank You Page can only vary between 40-60px. This is because after the --wp--preset--spacing--40, we only have --wp--preset--spacing--50: clamp(40px, calc(5vw + 10px), 60px);.

This means the mobile screen would be at 40px (which is acceptable per the previous discussion), and the maximum would be 60px (This hasn't been discussed yet, might need to see if this is acceptable since the Figma design specifies 70px, a difference of 10px)

👆 --wp--preset--spacing--50 is used eventually as per the feedback in the comment below.

Note

Only the Cover Block of the login page has been removed here. The Cover Block for the Thank You Page needs to be edited on the website.

@renintw renintw self-assigned this Oct 11, 2023
@renintw renintw added [Component] Theme Templates, patterns, CSS [Type] Bug Something isn't working labels Oct 11, 2023
@renintw renintw linked an issue Oct 11, 2023 that may be closed by this pull request
Mobile: Log in + Thank you screens #226
Closed
@renintw renintw added this to the Launch milestone Oct 11, 2023
Base automatically changed from fix/Thank-you-screen-dot-pattern to main October 12, 2023 07:23
@renintw
Copy link
Contributor Author

renintw commented Oct 12, 2023 •
edited
Loading

@jasmussen @marko-srb

Based on the first Note in the description above, if we change the margin between the dot pattern and text to 40px on mobile (currently at 30px), and the maximum margin on non-mobile to 60px (currently 70px as per Figma), would that be ok?

@marko-srb
Copy link

@renintw

Based on the first Note in the description above, if we change the margin between the dot pattern and text to 40px on mobile (currently at 30px), and the maximum margin on non-mobile to 60px (currently 70px as per Figma), would that be ok?

That works well, just tested in design. Feel free to proceed.

@renintw renintw force-pushed the fix/dots-pattern-on-mobile-log-in-thank-you-screens branch 2 times, most recently from 7e7ef5a to 4d0a967 Compare October 16, 2023 15:37
@renintw renintw force-pushed the fix/dots-pattern-on-mobile-log-in-thank-you-screens branch from 4d0a967 to 7b57c5f Compare October 16, 2023 15:44
@renintw
Copy link
Contributor Author

renintw commented Oct 16, 2023 •
edited
Loading

Thanks @marko-srb.

@adamwoodnz could you take a look at the PR? The question asked before has been clarified and I've updated the commit message and the results in the PR description. Thanks 🙏

image

adamwoodnz
adamwoodnz reviewed Oct 16, 2023
View reviewed changes
Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me. I notice that the dots are aligned left, so that more dots are added from the right as the screen gets wider. On small screens this looks a little unbalanced IMO

Have we considered positioning the dots centrally, possibly only on smaller screens where it's more noticeable?

background-position: left background-position: centre
localhost_8888_submit-a-wordpress-site_thanks_(Samsung Galaxy S20 Ultra) (1) localhost_8888_submit-a-wordpress-site_thanks_(Samsung Galaxy S20 Ultra) (2)

@renintw
Copy link
Contributor Author

renintw commented Oct 16, 2023

yeah, that change is from this PR and there are some discussions about this.

Have we considered positioning the dots centrally, possibly only on smaller screens where it's more noticeable?

@jasmussen do you think we should position the dots centrally for all sizes of screen or just smaller screen in this case?

@jasmussen
Copy link

Good eyes.

Most important thing: whatever we decide, we should do that everywhere. I believe that Marko designed it to be centered initially, though I suggested we should left-align because then the dots always line up with News on the left. But I do see that when we reach the mobile breakpoint, it's less elegant with the uneven padding left and right.

I'm happy to go full centered, so long as we do it consistently, and if Marko chimes in otherwise, I'll defer to him!

@renintw
Copy link
Contributor Author

renintw commented Oct 17, 2023

Thanks for the feedback. I've positioned the dot pattern centrally, let me know if any further tweaks are needed. If not, I'll merge it after a while.

@adamwoodnz
Copy link
Contributor

adamwoodnz commented Oct 17, 2023 •
edited
Loading

Also I notice we've got widows on mobile, we could add non breaking spaces to stop this

Screenshot 2023-10-18 at 8 31 02 AM

adamwoodnz
adamwoodnz approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@adamwoodnz adamwoodnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked at sizes of iPhone 12, Samsung Galaxy S20, iPad and desktop, and dots LGTM

@renintw renintw merged commit 4ec3624 into main Oct 18, 2023
1 check passed
@renintw renintw deleted the fix/dots-pattern-on-mobile-log-in-thank-you-screens branch October 18, 2023 07:03
@renintw renintw mentioned this pull request Oct 18, 2023
Closed
@renintw
Copy link
Contributor Author

renintw commented Oct 18, 2023 •
edited
Loading

white-space: nowrap would make it overflow, I've opened an issue to address this. See #255

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamwoodnz adamwoodnz adamwoodnz approved these changes

@marko-srb marko-srb Awaiting requested review from marko-srb

@StevenDufresne StevenDufresne Awaiting requested review from StevenDufresne

Assignees

@renintw renintw

Labels
[Component] Theme Templates, patterns, CSS [Type] Bug Something isn't working
Projects
WordPress.org Redesign: Showcase
Status: Done
Milestone
Launch
Development

Successfully merging this pull request may close these issues.

Mobile: Log in + Thank you screens
4 participants
@renintw @marko-srb @jasmussen @adamwoodnz